feat: add azure-advisor skill#2663
Conversation
Add the azure-advisor product-area router skill with a read-only review capability that uses available advisor_* MCP tools. Includes shared capability-routing and subscription-discovery references, trigger tests, and registers the skill in tests/skills.json.
There was a problem hiding this comment.
Pull request overview
Adds a new azure-advisor product-area router skill under plugin/skills/ with a single review capability, plus trigger tests and registration so it’s included in CI/integration scheduling.
Changes:
- Introduces the
azure-advisorskill (routerSKILL.md, shared references, andreview/capability workflow) with per-skillversion.jsonfor NBGV path filtering. - Adds trigger tests + snapshots for the new skill.
- Registers
azure-advisorintests/skills.jsonand adds it to the scheduled integration batch list.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/skills.json | Registers azure-advisor and includes it in the integration test schedule batch. |
| tests/azure-advisor/triggers.test.ts | Adds trigger/negative/edge-case tests for routing activation. |
| tests/azure-advisor/snapshots/triggers.test.ts.snap | Adds snapshots for extracted keywords and description-driven triggers. |
| plugin/skills/azure-advisor/version.json | Adds per-skill NBGV config via pathFilters. |
| plugin/skills/azure-advisor/SKILL.md | Adds the router skill frontmatter + capability routing table and constraints. |
| plugin/skills/azure-advisor/review/review.md | Adds the read-only “review” capability workflow, constraints, and error handling. |
| plugin/skills/azure-advisor/references/subscription-discovery.md | Adds shared subscription discovery guidance for all capabilities. |
| plugin/skills/azure-advisor/references/capability-routing.md | Adds shared guidance for resolving advisor_* MCP tools by capability. |
| plugin/skills/azure-advisor/README.md | Adds contributor-facing folder map and conventions for extending the skill. |
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Testing standards gap per tests/AGENTS.md:
- The "Should NOT Trigger" section has only 4 negative prompts (minimum is 5).
- None of the negative prompts test adjacent Azure services. The AGENTS.md guide explicitly requires prompts about "different Azure services not covered by this skill." Since the description's DO NOT USE FOR section mentions
azure-costandazure-diagnostics, boundary prompts like "analyze my Azure costs" or "troubleshoot my Azure subscription" are needed to verify the skill doesn't false-positive on adjacent domains. This matters because keyword extraction pulls "cost" and "diagnostics" into the trigger set from the DO NOT USE FOR text. - No
unit.test.tsfile is present. The testing checklist requires unit tests that verify the skill's content contains expected sections.
The eval CI check is also failing. Worth confirming whether it's related to this PR.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two things need fixing before this can merge:
-
No
unit.test.ts. Every other skill in the repo has one (the template includes it).tests/AGENTS.mdStep 5 and the checklist require unit tests that verify skill metadata and content sections. -
The
shouldNotTriggerPromptsarray has 4 entries; minimum is 5. None test adjacent Azure services. Since the skill'sDO NOT USE FORcalls outazure-costandazure-diagnostics, add boundary prompts like "analyze my Azure subscription costs" or "troubleshoot my Azure VM" to verify the skill doesn't false-positive on neighboring domains.
The Copilot review also flagged "IaaC" typos (should be "IaC") and .env* scanning in subscription-discovery.md. Worth fixing in the same pass.
…s, IaaC->IaC - Match MCP tools whose name contains advisor_ (e.g. azure-mcp-advisor_*) instead of requiring an advisor_ prefix, so Copilot CLI server-prefixed tools resolve correctly - Reword description to stop cost/diagnostics keyword bleed and expand shouldNotTriggerPrompts with adjacent-Azure-service boundaries (4 -> 9) - Regenerate triggers snapshot for updated description - Correct IaaC -> IaC across SKILL.md, README, references, and review docs - Harden .env* handling to read only AZURE_SUBSCRIPTION_ID line
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Correction on my earlier unit.test.ts feedback: no other skill in this repo actually has one (checked azure-compute, azure-deploy, azure-validate, azure-prepare, microsoft-foundry). The AGENTS.md documents it but nobody follows it. Withdrawing that as a blocker.
One remaining nit: README.md folder map table has duplicate rows for references/capability-routing.md and review/review.md. Each path appears twice with different purpose text. Consider collapsing each pair into a single row.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
My prior changes-request items are all resolved: negative prompts now cover adjacent-service boundaries (9 total), IaaC typos are fixed, and the README folder-map duplicates are collapsed.
One consistency issue remains: review.md references advisor_recommendation_summary by exact name in its constraints, but the rest of the skill routes by capability, not by hard-coded tool name. See inline comment.
The eval CI failure is pre-existing (azure-compute) and isn't related to this PR.
…eview Auto-discover every subscription the repo references (per-environment param/azd configs) and classify each into prod/staging/test/dev/other, running the Advisor review per subscription with results grouped by environment. Tenant-wide subscription-list enumeration becomes a widen-on-request/fallback path. Also route summary via the Aggregation/summary capability instead of a hard-coded tool name, and strengthen the case-insensitive trigger test.
| |------|---------| | ||
| | [SKILL.md](SKILL.md) | **Router.** Frontmatter (makes the skill discoverable) + a capability table that routes intent to a capability file. Keep it thin. | | ||
| | [references/capability-routing.md](references/capability-routing.md) | **Shared, capability-agnostic docs** reused by every capability. Don't duplicate these inside a capability. | | ||
| | [references/capability-routing.md](references/capability-routing.md) | How to resolve an `advisor_*` MCP tool by capability (catalog, recommendations, summary, IaC fix). | |
There was a problem hiding this comment.
Duplicate rows, keep single row for capability and review
| - **greenfield** — Advisor-informed guidance for new/empty subscriptions | ||
| - **cost** — Advisor cost-category optimization (coordinates with `azure-cost`) | ||
| - **reliability** — Advisor reliability-category reviews | ||
| - **governance** — Advisor operational-excellence / governance reviews |
There was a problem hiding this comment.
We can also add security and performance reviews to the roadmap here. But consider it as optional
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Prior changes-request items are all resolved: capability routing now references capability-routing.md instead of hard-coded tool names.
One formatting defect in the new commit: the Error Handling table in review.md has two rows merged onto a single line (the "Catalog call returns empty" and "Recommendation list 401/403" rows are separated by || instead of a newline). This breaks table rendering.
- Add resource-discovery reference and mandatory Step 1b to collect every repo resource type/group/id; apply scope in Steps 3-4 (filter or post-filter). - Reword SKILL.md description opener and add resource-scope + security/performance roadmap rows. - Add resource-discovery row to README folder map. - Drop whole-tenant catalog line from the chat summary (scope to customer resources, remove Tenant keyword). - Split merged error-handling table rows onto separate lines. - Update trigger snapshots.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 106786a (resource-scoping commit). The table formatting defect from my last review is fixed, and the new resource-discovery.md is well-structured with good security awareness around .env file handling.
One minor nit on the new file (non-blocking).
Bicep/ARM provider types are literal extractions; the Terraform azurerm_<kind> -> Microsoft.* mapping must be derived and can be non-obvious. When uncertain, fall back to the resource group filter rather than risk a wrong type filter that hides recommendations.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 59e0776 (Terraform type-mapping fallback note).
The distinction between literal extraction (Bicep/ARM) and derived mapping (Terraform) is the right framing. The azurerm_linux_function_app example illustrates why guessing is dangerous, and the fallback-to-resource-group-filter guidance prevents silent data loss from wrong type filters.
No new issues. My prior changes-requested items (negative prompts, IaC typos, README duplicates, hard-coded tool names, table formatting) are all resolved across the earlier commits.
jongio
dismissed
their stale review
Dismissing: this review was incorrectly posted as changes-requested due to an automation bug. The findings remain valid as comments.
| @@ -0,0 +1,67 @@ | |||
| --- | |||
| name: azure-advisor | |||
| description: "Azure Advisor reviews resources and provides recommendations using advisor_* MCP tools. WHEN: \"run an advisor review\", \"check my Azure advisor recommendations\", \"summarize advisor findings\", \"what does Advisor say about my subscription\", \"give me an advisor health check\", \"audit my Azure resources with Advisor\". USE FOR: read-only sweeps with catalog, recommendations, and IaC fixes. DO NOT USE FOR: changing resources, billing analysis (use azure-cost), or non-Advisor troubleshooting (use azure-diagnostics)." | |||
There was a problem hiding this comment.
You should tell the model which exact tool to use. If it's Azure MCP, mention that the tool is from Azure MCP.
There was a problem hiding this comment.
Done — description now specifies the advisor_* tools come from the Azure MCP server.
| |-----------|-------------|-----------| | ||
| | **review** | Run a holistic, read-only Advisor sweep across one subscription — or **all** subscriptions classified by environment (dev/staging/prod) — probing the catalog, pulling active recommendations, aggregating by category/impact, spotlighting high-impact items, and proposing IaC fix snippets. | [review](review/review.md) | | ||
|
|
||
| ### Roadmap (not yet implemented) |
There was a problem hiding this comment.
What does the "(not yet implemented)" mean?
There was a problem hiding this comment.
"(not yet implemented)" flags planned capabilities that aren't built yet — today only review ships. The list is a roadmap so future capabilities reuse the same shared references and get added as sibling folders. Happy to drop the roadmap section entirely if you'd prefer the skill only document what's live.
There was a problem hiding this comment.
You should remove this section and only keep the content that are ready.
|
We require all skills to have integration tests implemented as vally eval suites to express the scenarios they cover. Please read the tests/Readme.md file and follow the documentation + existing vally suite examples to implemented integration tests for your skill. Make sure to run your integration tests locally and refine the skill until they pass with meaningful results. |
Add evals/azure-advisor/eval.yaml covering review/recommendation/audit routing and response quality. Remove legacy Jest triggers test and snapshot. Drop README.md and reference Azure MCP server in SKILL.md description.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 3c79fb8 and f8ee26d (Vally eval migration + description trim).
Prior items are addressed: README.md removed per JasonYeMSFT's request, description now specifies Azure MCP tool sourcing, code fence language tags added to review.md, and the contributing callout removed since its target was deleted.
One gap in the new eval coverage: the old triggers.test.ts had 9 negative prompts for adjacent-service boundary testing. The new eval.yaml has zero negative stimuli. See inline comment.
| - type: completed | ||
| - type: output-not-matches | ||
| config: | ||
| pattern: "(?i)fatal error|unhandled exception|stack trace" |
There was a problem hiding this comment.
The old triggers.test.ts had 9 shouldNotTriggerPrompts covering adjacent-service boundaries (cost analysis, diagnostics, Key Vault, App Service, RBAC). This eval has zero negative stimuli.
The skill-invocation grader supports a disallowed config (the azure-compute eval uses it for boundary assertions). Consider adding 2-3 boundary stimuli, for example:
- name: "Cost query does not route to advisor"
prompt: "Analyze my Azure subscription costs and spending trends"
tags:
type: integration
tier: smoke
cost: llm
area: routing
graders:
- type: skill-invocation
config:
disallowed:
- azure-advisorWithout negative stimuli, there's no automated coverage to catch false-positive routing on the adjacent services called out in the skill's DO NOT USE FOR section (azure-cost, azure-diagnostics).
There was a problem hiding this comment.
Added 3 boundary stimuli — restores the false-positive routing coverage from the old shouldNotTriggerPrompts
|
|
||
| # ═══════════════════════════════════════════ | ||
| # Boundary (negative) routing — adjacent services from | ||
| # the skill's DO NOT USE FOR section must NOT route to azure-advisor. |
There was a problem hiding this comment.
The old triggers.test.ts covered 5 adjacent-service categories with 9 negative prompts. This adds 3 covering cost, diagnostics, and RBAC. The explicit DO NOT USE FOR services are covered. Key Vault and App Service boundaries from the old set aren't ported; consider adding them if false-positive routing on those surfaces is a concern.
|
|
||
| # ═══════════════════════════════════════════ | ||
| # Boundary (negative) routing — adjacent services from | ||
| # the skill's DO NOT USE FOR section must NOT route to azure-advisor. |
There was a problem hiding this comment.
The old triggers.test.ts covered 5 adjacent-service categories with 9 negative prompts. This adds 3 covering cost, diagnostics, and RBAC. The explicit DO NOT USE FOR services are covered. Key Vault and App Service boundaries from the old set aren't ported; consider adding them if false-positive routing on those surfaces is a concern.
There was a problem hiding this comment.
Ported the remaining two — added Key Vault and App Service boundary stimuli. All 5 adjacent-service categories from the old triggers.test.ts are now covered.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Previous comments addressed. The two new boundary stimuli (Key Vault, App Service) bring the negative-prompt count to 5, satisfying the AGENTS.md minimum. Structure matches the existing boundary tests. Nothing new to flag.
CI: the eval-job failure is in azure-compute lint, not this PR.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Previous comments addressed. The two new boundary stimuli (Key Vault, App Service) bring the negative-prompt count to 5, satisfying the AGENTS.md minimum. Structure matches the existing boundary tests. Nothing new to flag.
CI: the eval-job failure is in azure-compute lint, not this PR.
…zure-advisor eval
…onstraints from SKILL.md
There was a problem hiding this comment.
Incremental review of 515a56f and b512a45 (2 commits since my last review).
Changes address JasonYeMSFT's feedback:
- Removed dead environment block (executor loads skills by tag)
- Added per-stimulus skill: azure-advisor tags (matches azure-cost convention)
- Added 2 tool-call trajectory tests validating the agent invokes an advisor_ tool
- Removed duplicated Constraints section from SKILL.md (already expressed in description and capability-routing reference)
Trajectory tests use earlyTerminate with tool-call-match to stop as soon as the agent calls an advisor tool, and a count:8 safety cap. Structure is clean, graders are correct (skill-invocation + tool-calls + output-not-matches). No new issues.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 515a56f..b512a45 (2 commits since my last review).
Changes look good:
- Dead �nvironment block removed, per-stimulus skill: azure-advisor tags added (matches azure-compute convention).
- Tool-call trajectory tests validate the agent actually calls an �dvisor_ MCP tool, not just that the skill is invoked. The �arlyTerminate config with ool-call-count: 8 is a sensible cap.
- Duplicated Constraints section removed from SKILL.md. Those rules already live in
eview/review.md with more detail.
All prior feedback addressed. Eval coverage now spans routing (positive + negative boundary), behavior (tool-call trajectory), and response quality. No new issues.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Incremental review of 515a56f..b512a45 (2 commits since my last review).
Changes look good:
- Dead
environmentblock removed, per-stimulusskill: azure-advisortags added (matches azure-compute convention). - Tool-call trajectory tests validate the agent actually calls an
advisor_MCP tool, not just that the skill is invoked. TheearlyTerminateconfig withtool-call-count: 8is a sensible cap. - Duplicated Constraints section removed from SKILL.md. Those rules already live in
review/review.mdwith more detail.
All prior feedback addressed. Eval coverage now spans routing (positive + negative boundary), behavior (tool-call trajectory), and response quality. No new issues.
(Reposted to fix formatting from previous comment where backticks were corrupted.)
|
|
||
| # ── boundary-diagnostics ── | ||
| - name: "Diagnostics query does not route to advisor" | ||
| prompt: "My App Service keeps returning 500 errors, help me troubleshoot it" |
There was a problem hiding this comment.
I would recommend removing this test. It assumes a non-existent app service instance. The agent is smart and often it attempts to get information of the resource before doing anything. The test cannot proceed because the test resource doesn't exist.
|
|
||
| # ── boundary-rbac ── | ||
| - name: "RBAC query does not route to advisor" | ||
| prompt: "Grant my team Reader access to the production resource group" |
There was a problem hiding this comment.
I recommend removing all the negative tests.
| tier: full | ||
| cost: llm | ||
| area: behavior | ||
| earlyTerminate: '[{"type":"tool-call-match","toolPattern":"advisor_","argsPattern":".*"},{"type":"tool-call-count","count":8}]' |
There was a problem hiding this comment.
You need to use the tool-call-result early terminate condition. The tool-calls grader checks for tool results. too-call-match condition terminates run when the tool is called meaning they don't get their results so the vally grader will always fail.
Summary
Adds the
azure-advisorproduct-area router skill, ported and adapted to repo conventions.What's included
plugin/skills/azure-advisor/with a read-only review capability that uses whicheveradvisor_*MCP tools are available (routes by capability, not hard-coded tool names).tests/azure-advisor/(16 passing).tests/skills.json(skills array +0 12 * * 2-6integration batch).version.jsonadded; frontmatter uses0.0.0-placeholder(NBGV stamps at build).Validation